Result/Option layout guarantee clarifications #146509
Conversation
rustbot
added
S-waiting-on-review
RalfJung
force-pushed
the
res-opt-layout-guarantees
branch
from
fe2ac5c to
0b3d10d
Compare
RalfJung
force-pushed
the
res-opt-layout-guarantees
branch
from
0b3d10d to
57a8745
Compare
RalfJung
added
the
I-lang-nominated
traviscross
added
T-lang
|
@traviscross was it okay to assign you as reviewer here? This isn't meant to say anything new, just to clarify that when we say
then you can transmute between these two types, and that will preserve the values. |
|
Does this interact with #147185? |
|
I don't see how?
This is quite different from |
The notation used here (e.g. "transmute `t: T` to `Option<T>`") felt maybe a bit heavy, in the context of the library documentation, so let's elaborate this a bit. Also, in the `Option` docs, we talk about this being true for "the following types `T`", but it felt this caveat might get a bit lost in the next sentence that talks about the valid transmutations, so let's reiterate the caveat. While we're touching the line, we can improve: > The only difference is the implied semantics: This sentence was a bit awkward due to the mismatched plurality and not identifying the difference being spoken to. We'll reword this to make it more clear. We'll wrap to 80, since the existing text and most of the doc comments in these files are wrapped this way.
traviscross
force-pushed
the
res-opt-layout-guarantees
branch
from
30d3264 to
73f5fe7
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
library/core/src/option.rs
Outdated
| //! [`Option<T>`] has the same size, alignment, and [function call ABI] as `T`. | ||
| //! It is therefore sound to transmute `t: T` to `Option<T>` (which will produce `Some(t)`), and | ||
| //! to transmute `Some(t): Option<T>` to `T` (which will produce `t`). |
There was a problem hiding this comment.
In the same way as what we ended up revising on #140463, probably I think it's more clear to elaborate the description a bit rather than leaning too heavily on this t: T notation where it's implied that t is a value. Not that it'd be entirely foreign to Rust programmers, maybe, but it still seems more clear to just spell it out.
There was a problem hiding this comment.
I've revised the text here and in result.rs to elaborate this a bit.
library/core/src/option.rs
Outdated
| //! It is therefore sound to transmute `t: T` to `Option<T>` (which will produce `Some(t)`), and | ||
| //! to transmute `Some(t): Option<T>` to `T` (which will produce `t`). |
There was a problem hiding this comment.
The first sentence has the important caveat about this being true for "the following types T". In the second sentence, this caveat feels like it might get a bit lost to me, so it seems better to reiterate that.
There was a problem hiding this comment.
I've revised to do this.
|
@RalfJung: Thanks for the ping. This had fallen off of my radar. I've pushed some revisions (see the commit message and the review comments for details) and rebased. Let me know that the revisions look OK to you, then |
|
The commit you added looks good to me, thanks! @bors r=traviscross rollup |
bors
added
S-waiting-on-bors
Rollup of 6 pull requests Successful merges: - #146509 (Result/Option layout guarantee clarifications) - #147494 (std::thread spawn: Docs: Link to Builder::spawn; Make same.) - #147532 ( Port `#[cfg_attr]` to the new attribute parsing infrastructure) - #147783 (bootstrap: migrate to object 0.37) - #147792 (Fix autodiff incorrectly applying fat-lto to proc-macro crates ) - #147809 (rustdoc: Fix passes order so intra-doc links are collected after stripping passes) Failed merges: - #147813 (Warn on unused_attributes in uitests ) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #146509 - RalfJung:res-opt-layout-guarantees, r=traviscross Result/Option layout guarantee clarifications - It seems worth spelling out that this guarantee allows particular transmutes. - After the `Result` section was written, the `Option` section it referenced gained *more* guarantees, saying that `None` is represented as `[0u8; N]`. Those guarantees were not meant to apply to `Result`. Make the `Result` section more self-contained to make this more clear. - "Type has no fields" is unclear since there is no general definition of what the fields of some arbitrary type are. Replace that by a more accurate description of the actual check implemented [here](https://github.com/rust-lang/rust/blob/e379c7758667f900aaf5551c4553c7d4c121e3e1/compiler/rustc_lint/src/types.rs#L828-L838). r? `@traviscross`
6 participants
Resultsection was written, theOptionsection it referenced gained more guarantees, saying thatNoneis represented as[0u8; N]. Those guarantees were not meant to apply toResult. Make theResultsection more self-contained to make this more clear.r? @traviscross